-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds Velocity, ShearStress, StressDiagonal, observables #285
base: main
Are you sure you want to change the base?
Conversation
07e7b35
to
0a2dc66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good start, thanks @harveydevereux!
I suspect I knew a lot of the answers to my questions when we discussed this when you started, but maybe it's helpful coming to it with fresh eyes, so we can make things as clear as possible.
0a2dc66
to
3b4cb56
Compare
c581cb8
to
9f06af6
Compare
88bcb2b
to
28c5923
Compare
624fcf7
to
dc017d5
Compare
janus_core/processing/observables.py
Outdated
list[int] | ||
The indices for each self._components. | ||
""" | ||
return [self._components[c] for c in self.components] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just list(self._components.values())
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this may lead to duplicate indices. e.g. stress supports both "xy" and "yx" as self._components
. self.components
are the particular (say stress) components an observable "tracks" hence the need to access those particular values from the dict.
I've changed this so there is no allowed_components
property, just a _allowed_components
member to track this. Hopefully makes this clearer.
janus_core/processing/observables.py
Outdated
|
||
|
||
StressDiagonal = Stress(components=["xx", "yy", "zz"]) | ||
ShearStress = Stress(components=["xy", "yz", "zx"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't like that this isn't StressShear
to go with others. May also want StressHydrostatic
rather than Diagonal
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those are both good ideas,StressHydrostatic
sounds good, unless someone can think of better I'll go with it
janus_core/processing/observables.py
Outdated
for component in self.allowed_components.keys() - components.keys(): | ||
if component not in self.allowed_components: | ||
component_names = list(self._components.keys()) | ||
raise ValueError( | ||
f"'{component}' invalid, must be '{', '.join(component_names)}'" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blindly applying this isn't right.
You no longer need to check the if
, as that's done by the operation. component_names
is now just undefined and need to refer to the components which are not defined.
if any(self.allowed_components.keys() - components.keys()):
raise ValueError(
f"'{','.join(self.allowed_components.keys() - components.keys())}' invalid, must be in '{', '.join(self.allowed_components)}'"
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, unfortunately I did check the changes locally but then added them all together anyway... the code was odd due to the line length but this can be solved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's my problem really, I click "suggestion" because I'm too lazy to match the formatting and then it renders with "add this" even when it's invalid code. I should change the type to Python
janus_core/processing/observables.py
Outdated
if any(components - self._allowed_components.keys()): | ||
raise ValueError( | ||
f"'{components-self.allowed_components.keys()}'" | ||
" invalid, must be '{', '.join(self._components)}'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
" invalid, must be '{', '.join(self._components)}'" | |
f" invalid, must be '{', '.join(self._components)}'" |
janus_core/processing/observables.py
Outdated
return ( | ||
atoms.get_stress(include_ideal_gas=self.include_ideal_gas, voigt=True)[ | ||
self._index | ||
] | ||
sliced_atoms.get_stress( | ||
include_ideal_gas=self.include_ideal_gas, voigt=True | ||
) | ||
/ units.GPa | ||
) | ||
)[self._indices] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a bit messy and confusing, personally would prefer something more like:
stresses = sliced_atoms.get_stress(
include_ideal_gas=self.include_ideal_gas, voigt=True
) / units.GPa
return stresses[self._indices]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've split the index onto the return line as above, it is clearer.
janus_core/processing/observables.py
Outdated
if isinstance(self.atoms_slice, list): | ||
return len(self.atoms_slice) * self.dimension | ||
return slicelike_len_for(self.atoms_slice, self.n_atoms) * self.dimension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be best to move the list
case into slicelike_len_for
so we can have a clean interface here. Maybe selector_len
or something in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, but this also made me realise slicelike_len_for is not defined due to the import guarding (ultimately a circular import issue from before). @ElliottKasoar I have moved CorrelationKwargs
into correlator.py
so there is no longer an issue with Observable
and SliceLike
circular imports.
45dee78
to
1fdab69
Compare
if isinstance(index, (slice, range)): | ||
return (index.start, index.stop, index.step) | ||
|
||
return index |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth checking if index is a StartStopStep
(or any other valid input)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean throwing an error if it is not actually a SliceLike
?
i.e. these cases are no good but "run"
>>> slicelike_to_startstopstep([1,2])
[1, 2]
>>> slicelike_to_startstopstep((None, None, None))
(None, None, None)
its probably a good move
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do
def validate_slicelike(maybe_slicelike: SliceLike):
# ...
if isinstance(maybe_slicelike, (slice, range, int)):
return
if isinstance(maybe_slicelike, tuple) and len(maybe_slicelike) == 3:
start, stop, step = maybe_slicelike
if (
isinstance(start, Optional[int])
and isinstance(stop, Optional[int])
and isinstance(step, int)
):
return
raise ValueError(f"{maybe_slicelike} is not a valid SliceLike")
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that sort of thing would be great
Not really work for this PR as such, but how easy would it be to integrate/use these in post processing, rather than on-the-fly calculations? |
All it needs is to be called on some sequence of atoms objects. But if you are post-processing why not just use post-process? The only advantage of these is they extract the data from Atoms in this case |
Most significantly, I'm thinking about how easy it is to extend postprocess to observables we've defined, but aren't supported yet. At the moment we can only do RDFs and VAFs, so if we didn't calculate any other observable on the fly, we might still want to be able to use an It could also fit into unifying the interfaces e.g. for the VAF, depending on how their efficiencies compare, which would avoid questions about their consistency any time we make a change. |
apply review comments Co-authored-by: Jacob Wilkins <[email protected]>
5e25e15
to
47cf3bc
Compare
47cf3bc
to
874d849
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just so we know it's being run for side-effects.
validate slicelike return hint Co-authored-by: Jacob Wilkins <[email protected]>
This adds a Velocity observable capable of averaging over supplied components$\langle \sigma_{xy}\sigma_{xy}\rangle+\langle \sigma_{yz}\sigma_{yz}\rangle+\langle \sigma_{zx}\sigma_{zx}\rangle$ with the built in
x, y, z
and atom lists. The behaviour is abstracted so also for stress we can obtain shear stress correlationsShearStress === Stress(['xy', 'yz', 'zx'])
ci tests now test against post-process vaf, dependent on changes to it post_process uses full correlation, fix filter_atoms #284 .
I used the ShearStress observable to get experimental viscosity for LiF which could be an example perhaps along with the VAF.
still need to fix the docs/ update the developer guide.
perhaps we could have a helper function to create Velocity-Velocity correlations from a structure (say, creating correlation_kwargs for different atom types automatically). As it standsObservable
does not attach to a particular atoms object and so does not have this information at construction. Except implicitly viaatoms: list[int]
.Velocity can take a
SliceLike
orlist[int]
. So open-ended ranges can be specified i.e.slice(0, None)
, but also non-monotonic, decreasing or whatever sequences can be specified that cannot be represented as a slice.